Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: github-releases fill newDigest #10931

Closed
wants to merge 2 commits into from
Closed

feat: github-releases fill newDigest #10931

wants to merge 2 commits into from

Conversation

thepwagner
Copy link
Contributor

Changes:

This modifies the github-releases datasource to fill the newDigest field according to checksum files as release assets.

To accomplish this, the current release and digest are passed as parameters to getReleases():

  1. All <5KB assets are downloaded from the current release in an attempt to find the relevant checksum asset for the project (e.g. SHASUMS, SHASUMS.txt, ${artifact}.sha256).
  2. The corresponding checksum file is fetched from each release, and the appropriate updated checksum is returned.

Context:

I built the version of this functionality that I care about in https://github.com/thepwagner/action-update-dockerurl ; but the Renovate's regexManager handles 90% of the functionality I'm interested in! This algorithm is based on updatedHash from that project. I skipped the bit that fetches full asset files as that would be very expensive in the getReleases() context.

I'm using test cases from https://github.com/thepwagner/renovate-github-releases-digests .

Closes #7928

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@thepwagner
github-releases: log currentDigest
5bd43d7 
I'd like to fill `newDigest` in the  values returned here, and I think
the best approach to calculating the digest will require knowing the
previous value.

This initial commit wires the current version+digest (they were already
provided, but masked by types), and the logs the values.
@thepwagner
github-releases: digest support
bd330c5
@CLAassistant
Copy link

CLAassistant commented Jul 22, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@thepwagner
Copy link
Contributor Author

👋 I'm interested in some feedback on this injection point before I follow up with additional unit tests. A draft PR seemed easier than a discussion, please let me know if you'd strictly prefer discussions.

@thepwagner thepwagner mentioned this pull request Jul 22, 2021
Open
1 task
@rarkins
Copy link
Collaborator

rarkins commented Jul 23, 2021

@thepwagner thanks for the PR. It's a little hacky to populate newDigest from just one asset, but I like the idea of using the current digest to try to work out which asset is in use.

I'm not sure that getReleases() is the best interface, for a couple of reasons:

  • Ideally the return value of getReleases() is independent of the current repo, so that its results can be cached for all repos (if public)
  • It would be inefficient to do extra API requests to fetch digests for each release, when we usually only use 1-2 of them

Therefore we normally only return digests from getReleases() in cases where they are returned by the datasource beside the versions, and don't cost any extra network requests. I haven't used it in a long while and my memory is fuzzy but we have the getDigest() function which may be better suited for this, because it fetches the digest only for a version which will be used in a PR:

renovate/lib/datasource/types.ts

Line 63 in 7842ff7

getDigest?(config: DigestConfig, newValue?: string): Promise<string | null>;

The github-tags datasource uses that, for instance.

Could you take a look at that and confirm if I've understood your intention correctly and if getDigest() may be more suitable than getReleases()?

@thepwagner
Copy link
Contributor Author

thepwagner commented Jul 23, 2021 •
edited
Loading

@rarkins that sounds much more reasonable, thank you for the feedback!

I'd started out building on getDigest() as well, but stumbled upon the exact behaviour I expected if getReleases() filled newDigest.
I think my issues with the getDigest() paths were from unfamiliarity with the code: after some unrelated hacking on the kustomization manager I think I "get" versions versus digests now.

I'm going to close this PR as an abandoned path. If I have any questions about the getDigests() path, I'll take them to #7928 .

@thepwagner thepwagner closed this Jul 23, 2021
@thepwagner thepwagner deleted the renovate-7928 branch July 23, 2021 17:26
@thepwagner thepwagner mentioned this pull request Jul 23, 2021
Merged
6 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature-Request] RegexManager Checksum-Download Support
3 participants
@thepwagner @CLAassistant @rarkins